Skip to content

Kconfig: Fix FLASH_LOAD_{OFFSET,SIZE} issue when enabling BOOTLOADER_MCUBOOT in menuconfig + documentation and naming improvements #20740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Dec 9, 2019
Merged

Kconfig: Fix FLASH_LOAD_{OFFSET,SIZE} issue when enabling BOOTLOADER_MCUBOOT in menuconfig + documentation and naming improvements #20740

merged 4 commits into from
Dec 9, 2019

Conversation

@ulfalizer
Copy link
Contributor

@ulfalizer ulfalizer commented Nov 16, 2019 •
edited by ioannisg
Loading

Fixes: #20673

Main fix:

kconfig: Make FLASH_LOAD_OFFSET/SIZE non-configurable when DT is used

Having FLASH_LOAD_OFFSET and FLASH_LOAD_SIZE always configurable froze
their values at 0 when BOOTLOADER_MCUBOOT was enabled in menuconfig,
when instead the values from /chosen/zephyr,code-partition in devicetree
should be used. BOOTLOADER_MCUBOOT selects USE_CODE_PARTITION, which is
a flag to use the devicetree information.

To fix it, only make FLASH_LOAD_OFFSET and FLASH_LOAD_SIZE configurable
when USE_CODE_PARTITION is disabled. It looks like no configuration
files set them at the moment.

See the added documentation in
https://github.com/zephyrproject-rtos/zephyr/pull/20722 for an
explanation of why this happens. This bit novalisek in
https://github.com/zephyrproject-rtos/zephyr/issues/20673.

Fixes: #20673

Documentation improvements and cleanups:

kconfig: Improve USE_CODE_PARTITION prompt and help string

The prompt and help string for USE_CODE_PARTITION were too terse and
didn't make it clear that it's related to devicetree, which confused me.
Spell things out in more detail.

Unless the meaning of a symbol is completely obvious from context, aim
for at least a few sentences of help text. Think about what would be
confusing for someone coming at it without much context.

kconfig: Rename USE_CODE_PARTITION to USE_DT_CODE_PARTITION

USE_CODE_PARTITION is a bit vague as a symbol name ("use code partition
how?"). Rename it to USE_DT_CODE_PARTITION to make it clearer that it's
about devicetree.

This would break any third-party configuration files that set it, but
it'll generate an error since kconfig.py promotes warnings to errors, so
it's probably not a big deal.

kconfig: Factor out HAS_FLASH_LOAD_OFFSET dependency

Use a top-level 'if' instead of three separate 'depends on'. They're
exactly equivalent (top-level 'if's are just a shorthand for adding
'depends on' to each item within the 'if').

@ulfalizer
kconfig: Make FLASH_LOAD_OFFSET/SIZE non-configurable when DT is used
eac0c36 
Having FLASH_LOAD_OFFSET and FLASH_LOAD_SIZE always configurable froze
their values at 0 when BOOTLOADER_MCUBOOT was enabled in menuconfig,
when instead the values from /chosen/zephyr,code-partition in devicetree
should be used. BOOTLOADER_MCUBOOT selects USE_CODE_PARTITION, which is
a flag to use the devicetree information.

To fix it, only make FLASH_LOAD_OFFSET and FLASH_LOAD_SIZE configurable
when USE_CODE_PARTITION is disabled. It looks like no configuration
files set them at the moment.

See the added documentation in
zephyrproject-rtos#20722 for an
explanation of why this happens. This bit novalisek in
zephyrproject-rtos#20673.

Fixes: zephyrproject-rtos#20673

Signed-off-by: Ulf Magnusson <[email protected]>
@ulfalizer
kconfig: Improve USE_CODE_PARTITION prompt and help string
33bb397 
The prompt and help string for USE_CODE_PARTITION were too terse and
didn't make it clear that it's related to devicetree, which confused me.
Spell things out in more detail.

Unless the meaning of a symbol is completely obvious from context, aim
for at least a few sentences of help text. Think about what would be
confusing for someone coming at it without much context.

Signed-off-by: Ulf Magnusson <[email protected]>
@ulfalizer
kconfig: Rename USE_CODE_PARTITION to USE_DT_CODE_PARTITION
5f7373b 
USE_CODE_PARTITION is a bit vague as a symbol name ("use code partition
how?"). Rename it to USE_DT_CODE_PARTITION to make it clearer that it's
about devicetree.

This would break any third-party configuration files that set it, but
it'll generate an error since kconfig.py promotes warnings to errors, so
it's probably not a big deal.

Signed-off-by: Ulf Magnusson <[email protected]>
@ulfalizer
kconfig: Factor out HAS_FLASH_LOAD_OFFSET dependency
8a498bb 
Use a top-level 'if' instead of three separate 'depends on'. They're
exactly equivalent (top-level 'if's are just a shorthand for adding
'depends on' to each item within the 'if').

Signed-off-by: Ulf Magnusson <[email protected]>
@ulfalizer ulfalizer mentioned this pull request Nov 16, 2019
Closed
@ulfalizer ulfalizer requested a review from nvlsianpu November 16, 2019 00:53
@carlescufi carlescufi added the bug The issue is a bug, or the PR is fixing a bug label Nov 18, 2019
nvlsianpu
nvlsianpu approved these changes Nov 18, 2019
View reviewed changes
Copy link
Contributor

@nvlsianpu nvlsianpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look fine.
For future; Would be great if PR description was an abstract instead of commit messages conglomerate :D

ioannisg
ioannisg approved these changes Nov 18, 2019
View reviewed changes
Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

mbolivar
mbolivar approved these changes Nov 18, 2019
View reviewed changes
Copy link
Contributor

@mbolivar mbolivar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@novalisek
Copy link

novalisek commented Nov 18, 2019

see my comment at #20673 (comment) I think it is not working properly when enabling and disabling mcuboot again

@ioannisg
Copy link
Member

ioannisg commented Nov 19, 2019

Anyone outside Nordic would like to review this? @galak @MaureenHelm

@ulfalizer
Copy link
Contributor Author

ulfalizer commented Nov 20, 2019

see my comment at #20673 (comment) I think it is not working properly when enabling and disabling mcuboot again

Posted a comment there that explains it.

@ulfalizer ulfalizer mentioned this pull request Nov 20, 2019
Closed
@sslupsky
Copy link
Contributor

sslupsky commented Nov 25, 2019

Hi, I stumbled on this PR and issue #20673 . I had been struggling to get the code-partition setting in devicetree to actually relocate my code. I had to manually include

CONFIG_FLASH_BASE_ADDRESS=0x2000

in my proj.conf to locate my code past my boot loader (uf2-samd). Then I found this issue searching "code-partition". Turns out, adding

CONFIG_USE_CODE_PARTITION

works. So I added that to my board's defconfig.

The documentation about code-partition I referred to is here: https://github.com/zephyrproject-rtos/zephyr/blob/master/doc/guides/dts/flash_partitions.inc. I think it would be helpful if this page was updated to explain that CONFIG_USE_CODE_PARTITION needs to be set so that the code-partition devicetree setting is used.

This was referenced Nov 25, 2019
Closed
Closed
@ioannisg ioannisg added this to the v2.2.0 milestone Nov 26, 2019
@ioannisg
Copy link
Member

ioannisg commented Nov 26, 2019

Moving this to 2.2, to get reviews outside nordic

@nashif nashif merged commit fd9981a into zephyrproject-rtos:master Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbolivar mbolivar mbolivar approved these changes

@SebastianBoe SebastianBoe SebastianBoe approved these changes

@MaureenHelm MaureenHelm MaureenHelm approved these changes

@carlescufi carlescufi carlescufi approved these changes

@mnkp mnkp mnkp approved these changes

@lemrey lemrey Awaiting requested review from lemrey

@galak galak Awaiting requested review from galak

+2 more reviewers

@ioannisg ioannisg ioannisg approved these changes

@nvlsianpu nvlsianpu nvlsianpu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area: Boards area: Kconfig bug The issue is a bug, or the PR is fixing a bug

Projects

None yet

Milestone

v2.2.0

Development

Successfully merging this pull request may close these issues.

guiconfig not working properly?